Skip to content

Improvement sticky navbar #28840

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

Improvement sticky navbar #28840

wants to merge 7 commits into from

Conversation

mrtuvn
Copy link
Contributor

@mrtuvn mrtuvn commented Jun 22, 2020 •
edited
Loading

Description (*)

Continue work that was done by @divyajyothi5321 in #27050 after long inactivity
Add my additionals and validate code follow less standards

This PR bring styles change make main navigation menu sticky when scroll down when page height enough in theme luma
#1 Add new sticky lib less in magento ui library (can reusable later)
#2 Apply sticky for navigation in luma theme. For maintain compatible i decided should not add sticky in blank theme at the moment

Related Pull Requests

#27050
#26447

Fixed Issues (if relevant)

  1. Fixes Sticky header for navigation in mobile, desktop #26135

Manual testing scenarios (*)

  1. Set theme to luma
  2. Go to backend and create some item category for menu
  3. Go to frontend
  4. Scroll down to see menu sticky while scroll down

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Jun 22, 2020

Hi @mrtuvn. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE,
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

@mrtuvn
Copy link
Contributor Author

mrtuvn commented Jun 22, 2020

@krzksz Hi can you take a review this

@VladimirZaets VladimirZaets added the Priority: P4 No current plan to fix. Fixing can be deferred as a logical part of more important work. label Jun 24, 2020
Leland
Leland reviewed Jun 29, 2020
View reviewed changes
@mrtuvn mrtuvn force-pushed the improvement-navbar-with-sticky branch 2 times, most recently from 3e82f09 to 0f02e1c Compare June 29, 2020 07:41
@mrtuvn mrtuvn requested a review from Leland June 29, 2020 07:42
@mrtuvn
Copy link
Contributor Author

mrtuvn commented Jul 2, 2020

@ptylek @krzksz What do you think about my changes ? Can give me some review

Leland
Leland reviewed Jul 2, 2020
View reviewed changes
Leland
Leland reviewed Jul 2, 2020
View reviewed changes
@mrtuvn mrtuvn force-pushed the improvement-navbar-with-sticky branch 2 times, most recently from 6ef363f to d91f079 Compare July 2, 2020 16:02
@mrtuvn mrtuvn requested a review from Leland July 2, 2020 16:03
@mrtuvn mrtuvn force-pushed the improvement-navbar-with-sticky branch from d91f079 to bd2fa3f Compare July 2, 2020 23:40
@mrtuvn
Copy link
Contributor Author

mrtuvn commented Jul 3, 2020

@Leland thanks for your suggestion

woutk88
woutk88 suggested changes Jul 29, 2020
View reviewed changes
@mrtuvn mrtuvn force-pushed the improvement-navbar-with-sticky branch from bd2fa3f to fed8836 Compare July 29, 2020 08:40
@mrtuvn mrtuvn requested a review from woutk88 July 29, 2020 08:41
Leland
Leland reviewed Jul 29, 2020
View reviewed changes
@mrtuvn mrtuvn force-pushed the improvement-navbar-with-sticky branch from fed8836 to 0c8dc96 Compare July 30, 2020 02:14
@mrtuvn mrtuvn force-pushed the improvement-navbar-with-sticky branch from 0c8dc96 to b9b2426 Compare August 15, 2020 03:01
@mrtuvn mrtuvn force-pushed the improvement-navbar-with-sticky branch from b9b2426 to 9004f6f Compare October 1, 2020 00:59
@mrtuvn mrtuvn force-pushed the improvement-navbar-with-sticky branch from 9004f6f to 30faa80 Compare October 13, 2020 23:59
@mrtuvn mrtuvn force-pushed the improvement-navbar-with-sticky branch from 30faa80 to 26269b7 Compare January 21, 2021 14:33
@engcom-Alfa
Copy link
Contributor

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE, Static Tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@engcom-Charlie
Copy link
Contributor

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@engcom-Charlie
Copy link
Contributor

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE, Static Tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@engcom-Charlie
Copy link
Contributor

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@engcom-Charlie
Copy link
Contributor

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@engcom-Charlie
Copy link
Contributor

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@engcom-Charlie
Copy link
Contributor

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@engcom-Charlie
Copy link
Contributor

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@engcom-Charlie
Copy link
Contributor

engcom-Charlie commented Feb 22, 2022 •
edited
Loading

Hi @mrtuvn

Thank you for your contribution!

I have looked into this PR as many of the automation tests are getting failed continuously.
One of the most common error in test case failures is:

image

After analysis, I have found that; adding z-index(to 99)causing the failure of many test cases. Currently many of the functional test cases are broken and failed.

According to me, we need to rework on the given solution in this PR as many test cases are getting failed because of the solution given. Hence moving this PR to Changes Requested.

Thank you!

@mrtuvn
Copy link
Contributor Author

mrtuvn commented Feb 22, 2022

overlap menu above add to cart area may happen when scroll menu through area contain add to cart button. Cause un-able to click. That's happen with sticky menu.

@engcom-Charlie
Copy link
Contributor

Yes @mrtuvn it happens with sticky menu. So as I have mentioned in comment, too many test cases are getting failed and broken because of this solution.

In order to proceed with this PR, we have to update/adjust all those test cases which are failing because of this PR which I don’t think is a correct approach here.

@mrtuvn, do you want to propose any other solution for this issue?

Thank you!

@engcom-Charlie
Copy link
Contributor

On the basis of above findings and since we didn't got any reply on above comment, closing this PR.

Please reopen and update if you wish to continue.

Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Leland Leland Leland left review comments

@woutk88 woutk88 woutk88 approved these changes

@bgorski bgorski bgorski approved these changes

Assignees

@bgorski bgorski

@engcom-Alfa engcom-Alfa

@engcom-Charlie engcom-Charlie

@engcom-Hotel engcom-Hotel

Labels
Area: Design/Frontend Area: Lib/Frontend Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Release Line: 2.4 Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sticky header for navigation in mobile, desktop